[lldb] Clear thread name container before writing UTF8 bytes #134150
Conversation
|
@llvm/pr-subscribers-lldb Author: Jeremy Day (z2oh) Changes
llvm-project/llvm/lib/Support/ConvertUTFWrapper.cpp Lines 83 to 84 in 3bdf9a0 It's not clear to me why this function requires the output container to be empty instead of just overwriting it, but the callsite in This stack trace was captured from the lldb distributed in the Swift toolchain. The issue is easy to reproduce by resuming from a breakpoint twice in VS Code. I've verified that clearing out the container here fixes the assertion failure. Full diff: https://github.com/llvm/llvm-project/pull/134150.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
index a0d0f0ea0abc8..b2b66f2927644 100644
--- a/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp
@@ -192,6 +192,7 @@ const char *TargetThreadWindows::GetName() {
if (SUCCEEDED(GetThreadDescription(
m_host_thread.GetNativeThread().GetSystemHandle(), &pszThreadName))) {
LLDB_LOGF(log, "GetThreadDescription: %ls", pszThreadName);
+ m_name.clear();
llvm::convertUTF16ToUTF8String(
llvm::ArrayRef(reinterpret_cast<char *>(pszThreadName),
wcslen(pszThreadName) * sizeof(wchar_t)),
|
|
@z2oh Would you mind cherry-picking this to https://github.com/swiftlang/llvm-project/tree/stable/20240723? |
|
Thank you! FYI, I had created a bug report (which can now be closed) for this problem, but forgot to add that information to the Swift forum post, sorry for that. |
|
I was also surprised by the precondition, which looks unnecessary and, more importantly, very unexpected, to me. And it's not mentioned in the documentation: Should I prepare a PR to mention the precondition in the documentation? |
|
Cherrypick is up: swiftlang#10415. Thanks @JDevlieghere! @ktraunmueller, ah I should've checked for an issue first, you had already done the investigation! I'm in favor of the documentation change you propose. I'd also be in favor of just removing the assertion, but it's very possible I'm overlooking some reason why it's there in the first place (and it has been there for 12 years now). |
|
@compnerd weighed in that assertions are always there for a reason, and I would generally agree on that 100%, but this precondition just doesn't seem to make sense, so I would agree that removing it would seem preferrable in this particular case. I can't see how that would create harm. |
|
Famous last words, haha |
|
I think that a fully symbolicated stack trace would be very helpful. It might explain the reason for the precondition. |
|
I captured one above: We are reusing the
|
|
UTF-8 is a multibyte encoding, and if there is existing content in the output string, the generated result may not be a valid string, if you passed in a buffer with |
…4150) `llvm::convertUTF16ToUTF8String` opens with an assertion that the output container is empty: https://github.com/llvm/llvm-project/blob/3bdf9a08804a5b424fd32fef3b0089f3a6db839d/llvm/lib/Support/ConvertUTFWrapper.cpp#L83-L84 It's not clear to me why this function requires the output container to be empty instead of just overwriting it, but the callsite in `TargetThreadWindows::GetName` may reuse the container without clearing it out first, resulting in an assertion failure: ``` # Child-SP RetAddr Call Site 00 000000d2`44b8ea48 00007ff8`beefc12e ntdll!NtTerminateProcess+0x14 01 000000d2`44b8ea50 00007ff8`bcf518ab ntdll!RtlExitUserProcess+0x11e 02 000000d2`44b8ea80 00007ff8`bc0e0143 KERNEL32!ExitProcessImplementation+0xb 03 000000d2`44b8eab0 00007ff8`bc0e4c49 ucrtbase!common_exit+0xc7 04 000000d2`44b8eb10 00007ff8`bc102ae6 ucrtbase!abort+0x69 05 000000d2`44b8eb40 00007ff8`bc102cc1 ucrtbase!common_assert_to_stderr<wchar_t>+0x6e 06 000000d2`44b8eb80 00007fff`b8e27a80 ucrtbase!wassert+0x71 07 000000d2`44b8ebb0 00007fff`b8b821e1 liblldb!llvm::convertUTF16ToUTF8String+0x30 [D:\r\_work\swift-build\swift-build\SourceCache\llvm-project\llvm\lib\Support\ConvertUTFWrapper.cpp @ 88] 08 000000d2`44b8ec30 00007fff`b83e9aa2 liblldb!lldb_private::TargetThreadWindows::GetName+0x1b1 [D:\r\_work\swift-build\swift-build\SourceCache\llvm-project\lldb\source\Plugins\Process\Windows\Common\TargetThreadWindows.cpp @ 198] 09 000000d2`44b8eca0 00007ff7`2a3c3c14 liblldb!lldb::SBThread::GetName+0x102 [D:\r\_work\swift-build\swift-build\SourceCache\llvm-project\lldb\source\API\SBThread.cpp @ 432] 0a 000000d2`44b8ed70 00007ff7`2a3a5ac6 lldb_dap!lldb_dap::CreateThread+0x1f4 [S:\SourceCache\llvm-project\lldb\tools\lldb-dap\JSONUtils.cpp @ 877] 0b 000000d2`44b8ef10 00007ff7`2a3b0ab5 lldb_dap!`anonymous namespace'::request_threads+0xa6 [S:\SourceCache\llvm-project\lldb\tools\lldb-dap\lldb-dap.cpp @ 3906] 0c 000000d2`44b8f010 00007ff7`2a3b0fe8 lldb_dap!lldb_dap::DAP::HandleObject+0x1c5 [S:\SourceCache\llvm-project\lldb\tools\lldb-dap\DAP.cpp @ 796] 0d 000000d2`44b8f130 00007ff7`2a3a8b96 lldb_dap!lldb_dap::DAP::Loop+0x78 [S:\SourceCache\llvm-project\lldb\tools\lldb-dap\DAP.cpp @ 812] 0e 000000d2`44b8f1d0 00007ff7`2a4b5fbc lldb_dap!main+0x1096 [S:\SourceCache\llvm-project\lldb\tools\lldb-dap\lldb-dap.cpp @ 5319] 0f (Inline Function) --------`-------- lldb_dap!invoke_main+0x22 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 78] 10 000000d2`44b8fb80 00007ff8`bcf3e8d7 lldb_dap!__scrt_common_main_seh+0x10c [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl @ 288] 11 000000d2`44b8fbc0 00007ff8`beefbf6c KERNEL32!BaseThreadInitThunk+0x17 12 000000d2`44b8fbf0 00000000`00000000 ntdll!RtlUserThreadStart+0x2c ``` This stack trace was captured from the lldb distributed in the Swift toolchain. The issue is easy to reproduce by resuming from a breakpoint twice in VS Code. I've verified that clearing out the container here fixes the assertion failure. (cherry picked from commit be3abfc)
…ead-name-container [🍒] [lldb] Clear thread name container before writing UTF8 bytes (llvm#134150)
llvm::convertUTF16ToUTF8Stringopens with an assertion that the output container is empty:llvm-project/llvm/lib/Support/ConvertUTFWrapper.cpp
Lines 83 to 84 in 3bdf9a0
It's not clear to me why this function requires the output container to be empty instead of just overwriting it, but the callsite in
TargetThreadWindows::GetNamemay reuse the container without clearing it out first, resulting in an assertion failure:This stack trace was captured from the lldb distributed in the Swift toolchain. The issue is easy to reproduce by resuming from a breakpoint twice in VS Code.
I've verified that clearing out the container here fixes the assertion failure.